[workerd-cxx] Add kj::Maybe type to kj-rs #42
Conversation
LordGoatius
force-pushed
the
jostler/2025-06-30-kj-maybe
branch
6 times, most recently
from
e674fb2 to
2944364
Compare
kj-rs/maybe.rs
Outdated
| if self.is_none() { | ||
| write!(f, "kj::None") | ||
| } else { | ||
| write!(f, "kj::Some({:?})", unsafe { self.some.assume_init_ref() }) |
There was a problem hiding this comment.
Nit for discussion: Perhaps these should be lowercased, like kj::none (the value instead of its type kj::None) and kj::some() (the function instead of kj::Some, which doesn't actually exist)?
There was a problem hiding this comment.
I tried to have it more accurately represent Rust naming conventions here, with capitalized variants, as if it was a Rust enum. That's ideally the way we want to work with Maybe, but it does raise questions about when to use what conventions, where, and how. Keeping it consistent with C++ is probably worth it, since that's where we're migrating from, but should that change in the future?
There was a problem hiding this comment.
I think that makes sense, if kj::None and kj::Some happen to be valid Rust code.
There was a problem hiding this comment.
It currently isn't, but it may be possible to add these "constructors" to the Maybe type to allow creating it from Rust. Creating an Own from Rust was something that we decided against, but I think it might make sense to allow this for Maybe.
There was a problem hiding this comment.
I suppose we'll need to if we want to be able to call C++ functions which accept Maybe as parameters? We'll almost certainly want to do so at some point.
Maybe we leave that for a follow-up PR -- I don't want to block this on perfection.
There was a problem hiding this comment.
We should be able to do that already, I'll add some tests to make sure. I think if we have a C++ object we want to wrap in a maybe and return from rust, that's something we should be able to do. We should already be able to do that by wrapping it into an option, and converting that, but that could get to be an annoying step.
LordGoatius
force-pushed
the
jostler/2025-06-30-kj-maybe
branch
4 times, most recently
from
b3eb1b5 to
e2ae34a
Compare
kj-rs/maybe.rs
Outdated
| if self.is_none() { | ||
| write!(f, "kj::None") | ||
| } else { | ||
| write!(f, "kj::Some({:?})", unsafe { self.some.assume_init_ref() }) |
There was a problem hiding this comment.
I suppose we'll need to if we want to be able to call C++ functions which accept Maybe as parameters? We'll almost certainly want to do so at some point.
Maybe we leave that for a follow-up PR -- I don't want to block this on perfection.
| } | ||
| } | ||
| Maybe!( | ||
| u8, u16, u32, u64, u128, usize, i8, i16, i32, i64, i128, isize, bool |
There was a problem hiding this comment.
Just curious, is the macro incompatible with f32, f64?
There was a problem hiding this comment.
Yeah, clippy complains about directly comparing floating point values. I decided to just remove them, since the rest of the test should sufficiently handle testing that anyways, and the macro is just extra.
LordGoatius
force-pushed
the
jostler/2025-06-30-kj-maybe
branch
from
e2ae34a to
a41b1e8
Compare
LordGoatius
force-pushed
the
jostler/2025-06-30-kj-maybe
branch
from
a41b1e8 to
a9fe832
Compare
|
|
||
| out.include.utility = true; | ||
| out.include.kj_rs = true; | ||
| writeln!(out, "static_assert(!::std::is_pointer<{}>::value, \"Maybe<T*> is not supported in workerd-cxx\");", inner); |
There was a problem hiding this comment.
this would be a great place to put a comment explaining what's missing/needs to be done.
No description provided.